[Server] Add ToolExecutionExceptionInterface for custom tool error handling #73

MartkCz · 2025-09-17T11:26:52Z

Motivation and Context

The current tool execution system lacks a standardized way for handling tool-specific errors. This change provides a consistent way for tools to report errors.

How Has This Been Tested?

Added unit tests in ToolCallerTest.php

Breaking Changes

No breaking changes.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update

Checklist

I have read the MCP Documentation
My code follows the repository's style guidelines
New and existing tests pass locally
I have added appropriate error handling
I have added or updated documentation as needed

Additional context

The implementation includes modifications to:

ToolCaller and ToolCallerInterface for exception handling integration
ToolChain for proper error propagation
New ToolExecutionExceptionInterface defining the contract for custom tool error handling

Usage

class McpElementsException extends RuntimException implements ToolExecutionExceptionInterface { ... }

class McpElements
{
	#[McpTool(name: 'calculate')]
	public function calculate(float $a, float $b, string $operation): float|string
	{
		$op = strtolower($operation);

		switch ($op) {
			case 'add':
				$result = $a + $b;
				break;
			case 'subtract':
				$result = $a - $b;
				break;
			case 'multiply':
				$result = $a * $b;
				break;
			case 'divide':
				if (0 == $b) {
					throw new McpElementsException(['Division by zero.']);
				}
				$result = $a / $b;
				break;
			default:
				throw new McpElementsException(["Unknown operation '{$operation}'. Supported: add, subtract, multiply, divide."]);
		}

		if (!$this->config['allow_negative'] && $result < 0) {
			throw new McpElementsException(['Negative results are disabled.']);
		}

		return round($result, $this->config['precision']);
	}
}

chr-hertel · 2025-09-20T19:31:37Z

Hi @MartkCz,
I think you're right, we should at least document how users should deal with that kind of situation, but I disagree on the naming here.

The ReferenceHandlerInterface is rather generic and does/should not care/know about the concrete element implementation of tools or prompts, etc. In your example you're implementing the exception even in a generic way again with an McpElementsException - a bit confusing tbh.

Currently the ReferenceHandlerInterface states already, that you should throw a RegistryException "if execution fails". So the use case is covered, only the exception is not tailored.

I would prefer, instead of creating a new interface to implement, you create a ReferenceExecutionException extends RegistryException or ElementsExecutionException or similar - which is more generic than tool specific - and that new one replacing the RegistryException on the interface. WDYT?

MartkCz · 2025-09-22T12:07:04Z

@chr-hertel Thanks for your time.

I removed duplicate logging in the CallToolHandler (all logging is already here: https://github.com/modelcontextprotocol/php-sdk/blob/main/src/Capability/Tool/ToolCaller.php#L51-L76
).
I fixed https://github.com/modelcontextprotocol/php-sdk/blob/main/tests/Server/RequestHandler/CallToolHandlerTest.php#L117-L145
— when a tool is not found, the code should not be -32603 (INTERNAL_ERROR), but -32602 (INVALID_PARAMS), see https://modelcontextprotocol.io/specification/2025-06-18/server/tools#error-handling
.
I think it creates a mess because RegistryException and ReferenceExecutionException must be wrapped into ToolCallException and then unwrapped to determine whether it’s a protocol error or an execution error, since each is handled differently. On top of that, there’s a separate protocol error ToolNotFoundException. In my opinion, the best approach would be to split them into protocol and execution exceptions and throw those instead of ToolCallException and ToolNotFoundException.

interface ReferenceExceptionInterface {}

final class ExecutionReferenceException extends \RuntimException implements ReferenceExceptionInterface {} // business errors

abstract class ProtocolReferenceException extends \LogicException implements ReferenceExceptionInterface {} // unknown tools, invalid arguments, server errors

final class ToolNotFoundReferenceException extends ProtocolReferenceException {}

final class InvalidParamsReferenceException extends ProtocolReferenceException {}

final class InternalErrorReferenceException extends ProtocolReferenceException {}

This is just a proposal — there wouldn’t necessarily have to be so many exception classes inheriting from ProtocolReferenceException, but I wanted to preserve throwing ToolNotFoundException. The naming is also not final.

and then this is simplified from this:

try {
   $result = $this->referenceHandler->handle($toolReference, $arguments);
   /** @var TextContent[]|ImageContent[]|EmbeddedResource[]|AudioContent[] $formattedResult */
   $formattedResult = $toolReference->formatResult($result);

   $this->logger->debug('Tool executed successfully', [
       'name' => $toolName,
       'result_type' => \gettype($result),
   ]);

   return new CallToolResult($formattedResult);
} catch (RegistryException $e) {
   throw new ToolCallException($request, $e);
} catch (\Throwable $e) {
   if ($e instanceof ToolCallException) {
       throw $e;
   }

   $this->logger->error('Tool execution failed', [
       'name' => $toolName,
       'exception' => $e->getMessage(),
       'trace' => $e->getTraceAsString(),
   ]);

   throw new ToolCallException($request, RegistryException::internalError('Error while executing tool', $e));
}

to this:

try {
	$result = $this->referenceHandler->handle($toolReference, $arguments);
	/** @var TextContent[]|ImageContent[]|EmbeddedResource[]|AudioContent[] $formattedResult */
	$formattedResult = $toolReference->formatResult($result);

	$this->logger->debug('Tool executed successfully', [
		'name' => $toolName,
		'result_type' => \gettype($result),
	]);

	return new CallToolResult($formattedResult);
} catch (ReferenceExceptionInterface $e) {
	throw $e;
} catch (\Throwable $e) {
	$this->logger->error('Tool execution failed', [
		'name' => $toolName,
		'exception' => $e->getMessage(),
		'trace' => $e->getTraceAsString(),
	]);

	throw new InternalErrorReferenceException($request, $e);
}

and this:

try {
    $content = $this->toolCaller->call($message);
} catch (ToolNotFoundException $exception) {
    return Error::forInvalidParams($exception->getMessage(), $message->getId());
} catch (ToolCallException $exception) {
    $registryException = $exception->registryException;

    if ($registryException instanceof ReferenceExecutionException) {
        return new Response($message->getId(), CallToolResult::error(array_map(
            fn (string $message): TextContent => new TextContent($message),
            $registryException->messages,
        )));
    }

		return new Error($message->getId(), $registryException->getCode(), $registryException->getMessage());
}

to this:

try {
	$content = $this->toolCaller->call($message);
} catch (ProtocolReferenceException $exception) {
	return new Error($message->getId(), $exception->getCode(), $exception->getMessage());
} catch (ExecutionReferenceException $exception) {
	return new Response($message->getId(), CallToolResult::error(array_map(
		fn (string $message): TextContent => new TextContent($message),
		$exception->messages,
	)));
}

chr-hertel · 2025-09-22T22:13:05Z

@MartkCz Thanks for the follow-up and the detailed explanation! 🙏
I think your proposal sounds & looks reasonable to me 👍

btw, this needs a rebase - and please make sure to have a squashed, signed commit please

chr-hertel · 2025-11-11T21:14:42Z

Hi @MartkCz - do you see your concerns also addressed with #124?

MartkCz · 2025-11-13T11:29:19Z

Hi @chr-hertel , sorry, but I didn't have time to finish the PR.

At a quick glance it looks like a sufficient solution, the only question is whether it's really necessary to log errors from ToolCallException. In my experience, they were never needed, and in the worst case you can log directly in the McpTool method.

If I take an example where the AI/human calls divideNumbers with $b as 0 and McpTool responds that you can't divide by zero, I would expect it to handle that and provide a different number or ask for a different one. Otherwise, I would end up with Sentry full of logs that I can’t and don’t want to deal with, because it’s not an application error.

Logging is probably fine during debugging, but I would keep it at most at debug/info severity.

Thanks for mention.

chr-hertel · 2025-11-13T21:47:51Z

Thanks @MartkCz for the fast feedack - referring to this one, right?

php-sdk/src/Server/Handler/Request/CallToolHandler.php

Lines 77 to 85 in 7a3b473

    
           } catch (ToolCallException $e) { 
        
               $this->logger->error(\sprintf('Error while executing tool "%s": "%s".', $toolName, $e->getMessage()), [ 
        
                   'tool' => $toolName, 
        
                   'arguments' => $arguments, 
        
               ]); 
        
               $errorContent = [new TextContent($e->getMessage())]; 
        
               return new Response($request->getId(), CallToolResult::error($errorContent));

Add ToolExecutionExceptionInterface for custom tool error handling

8543966

MartkCz requested review from CodeWithKyrian, Nyholm and chr-hertel as code owners September 17, 2025 11:26

chr-hertel changed the title ~~Add ToolExecutionExceptionInterface for custom tool error handling~~ [Server] Add ToolExecutionExceptionInterface for custom tool error handling Sep 21, 2025

chr-hertel added the Server Issues & PRs related to the Server component label Sep 21, 2025

replace interface with class

4a33471

ineersa mentioned this pull request Oct 5, 2025

[Server] Structured content return and tool error #93

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Server] Add ToolExecutionExceptionInterface for custom tool error handling #73

[Server] Add ToolExecutionExceptionInterface for custom tool error handling #73

MartkCz commented Sep 17, 2025

Uh oh!

chr-hertel commented Sep 20, 2025

Uh oh!

MartkCz commented Sep 22, 2025

Uh oh!

chr-hertel commented Sep 22, 2025

Uh oh!

chr-hertel commented Nov 11, 2025

Uh oh!

MartkCz commented Nov 13, 2025

Uh oh!

chr-hertel commented Nov 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

[Server] Add ToolExecutionExceptionInterface for custom tool error handling #73

Are you sure you want to change the base?

[Server] Add ToolExecutionExceptionInterface for custom tool error handling #73

Conversation

MartkCz commented Sep 17, 2025

Motivation and Context

How Has This Been Tested?

Breaking Changes

Types of changes

Checklist

Additional context

Usage

Uh oh!

chr-hertel commented Sep 20, 2025

Uh oh!

MartkCz commented Sep 22, 2025

Uh oh!

chr-hertel commented Sep 22, 2025

Uh oh!

chr-hertel commented Nov 11, 2025

Uh oh!

MartkCz commented Nov 13, 2025

Uh oh!

chr-hertel commented Nov 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants